-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[BREAKING] Python: Validate checkpoint configuration consistency in sub-workflows, adjust workflow validation error #3756
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds explicit validation to prevent undefined checkpoint/restore behavior when a checkpoint-enabled parent workflow contains non-checkpointed sub-workflows, and simplifies workflow validation typing by replacing the enum with string-literal validation types.
Changes:
- Add build-time validation in
WorkflowBuilder.build()to require checkpoint configuration consistency forWorkflowExecutorchildren when the parent has checkpointing enabled. - Add runtime validation in
Workflow._run_core()whencheckpoint_storageis supplied viaworkflow.run(...). - Replace
ValidationTypeEnumwith aValidationTypestring-literal type and update tests/samples accordingly.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| python/packages/core/agent_framework/_workflows/_workflow_builder.py | Build-time validation for checkpointing consistency across nested workflows. |
| python/packages/core/agent_framework/_workflows/_workflow.py | Adds has_checkpointing and runtime validation for sub-workflow checkpoint configuration. |
| python/packages/core/agent_framework/_workflows/_validation.py | Replaces enum-based validation type with Literal[...] string types and updates error formatting. |
| python/packages/core/agent_framework/_workflows/init.py | Updates public exports to expose ValidationType instead of ValidationTypeEnum. |
| python/packages/core/tests/workflow/test_validation.py | Updates assertions to use string validation types instead of enums. |
| python/packages/core/tests/workflow/test_sub_workflow.py | Updates checkpointing-related sub-workflow construction to satisfy new validation rules. |
| python/packages/core/tests/workflow/test_checkpoint_configuration.py | Adds coverage for build-time/runtime checkpoint mismatch validation in nested workflows. |
| python/samples/getting_started/workflows/checkpoint/sub_workflow_checkpoint.py | Updates sample to explicitly configure checkpoint storage in sub-workflow builder. |
python/packages/core/tests/workflow/test_checkpoint_configuration.py
Outdated
Show resolved
Hide resolved
python/packages/core/tests/workflow/test_checkpoint_configuration.py
Outdated
Show resolved
Hide resolved
python/packages/core/tests/workflow/test_checkpoint_configuration.py
Outdated
Show resolved
Hide resolved
| from ._workflow_executor import WorkflowExecutor | ||
|
|
||
| for executor in self.executors.values(): | ||
| if isinstance(executor, WorkflowExecutor) and not executor.workflow.has_checkpointing: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if I am following. More specifically, I have the following questions:
- Why do we require all subworkflow to have checkpoint storage configured with the workflow instances?
- Have we considered passing the parent's storage to the sub workflow at
run? - What happens if the subworkflow has a different storage setup than the parent workflow?
|
Don't need this specific behavior as part of subworkflows. |
Motivation and Context
When a parent workflow has checkpointing enabled but a sub-workflow does not, the checkpoint/restore behavior is undefined. The current implementation in
WorkflowExecutor.on_checkpoint_restore()manually rehydrates sub-workflow state by accessing the private_runner_contextobject, with a TODO acknowledging this is a temporary solution. ThisPR adds validation to detect the mismatch early and error clearly, rather than letting developers hit undefined behavior at runtime.
Breaking changes
Checkpoint configuration validation: Previously, having a parent workflow with checkpointing and a sub-workflow without it was silently accepted but produced undefined checkpoint/restore
behavior. Now this raises
WorkflowValidationErrorwithvalidation_type="checkpoint_configuration"at build time (or at runtime ifcheckpoint_storageis provided only viaworkflow.run()).ValidationTypeEnumremoved:WorkflowValidationError.validation_typeis now aValidationTypestring literal (e.g.,"edge_duplication","checkpoint_configuration") instead of aValidationTypeEnummember. Compare directly against strings instead of enum values.Migration for checkpoint validation: pass
checkpoint_storageto every sub-workflow'sWorkflowBuilderwhen the parent workflow uses checkpointing.Description
Contribution Checklist